Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rETH #31

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

rETH #31

wants to merge 32 commits into from

Conversation

julienmartinlevrai
Copy link
Contributor

@julienmartinlevrai julienmartinlevrai commented Jun 23, 2022

Set to be merged after #30

@julienmartinlevrai julienmartinlevrai self-assigned this Jun 23, 2022
@julienmartinlevrai julienmartinlevrai marked this pull request as ready for review July 7, 2022 12:04
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
src/rETHCurveUniv3Callee.sol Outdated Show resolved Hide resolved
src/rETHCurveUniv3Callee.sol Outdated Show resolved Hide resolved
@@ -950,7 +952,7 @@ contract SimulationTests is DSTest {
lpDaiEthClip.take(auctionId, amt, max, cheAddr, data);
}

function testTakeLpDaiEthNoProfit() public {
function testTakeLpDaiEthNoProfit() private { // most LP tokens are getting offboarded
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that we have these tests replaced in a pending PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You did a really great job of replacing these tests.

src/test/Simulation.t.sol Outdated Show resolved Hide resolved
src/test/Simulation.t.sol Outdated Show resolved Hide resolved
@julienmartinlevrai
Copy link
Contributor Author

@talbaneth thank you for your review. I will be addressing it shortly.

Copy link
Contributor

@talbaneth talbaneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes ok, tests pass.

LGTM for deploying.
Don't forget to add the address to addresses.json.

Copy link
Contributor

@godsflaw godsflaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@talbaneth
Copy link
Contributor

I see test_bigAmtWithComplexPath is failing.
Current reth market price is 1,337.

The test is trying to take 3K reth at price of 970 reth/dai with greater than zero profit. Was still failing for 2K but succeeded at 1.5K reth.
Then I tried at 1172 reth/dai (~10% lower than current reth price), failed at 1.5K, suceeded at 1K.
So seems like the liquidity is not great, but still good enough to backstop.
For now I'll change the test to use 1K reth.

@talbaneth
Copy link
Contributor

@julienmartinlevrai @godsflaw please check and approve to merge

Copy link
Contributor Author

@julienmartinlevrai julienmartinlevrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • contract deployed at mainnet address 0x7cdAb0fE16efb1EFE89e53B141347D7F299d6610
  • contract code verified on Etherscan
  • verified code matches the code of this PR
  • addresses in constructor parameters correspond to the right contracts
  • no libraries
  • this contract does not use any type of auth

LGTM (I can’t approve my own PR though)

Copy link
Contributor Author

@julienmartinlevrai julienmartinlevrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants